Coverage remained the same at 82.987% when pulling 63416d2 on jtibbertsma:add-new-api-register-renderer into 5e5b92e on shakacode:master.

@jtibbertsma Very intriguing!

1. Can you modify the example in spec/dummy to demonstrate this? Or make spec/dummy2?
2. Alternately a PR to https://github.com/shakacode/react-webpack-rails-tutorial/ would be great to demonstrate this. The PR can refer to your fork.

@robwise @alexfedoseev We'll have to give this a close look in the coming weeks. However, please stay focused on F&G v2! We can potentially leverage this when we make a big push for performance after v2 is completed!

@justin808 Sure, I'll work on a pr for react-webpack-rails-tutorial this weekend.

@jtibbertsma I'm looking forward to seeing the changes!

@jtibbertsma Any update?

@justin808 I've been a bit busy this week. I should have some time to work on it today.

Coverage remained the same at 82.987% when pulling 79d2e87 on jtibbertsma:add-new-api-register-renderer into 7fecce1 on shakacode:master.

@jtibbertsma Could have a simple example in /spec/dummy along with some simple tests of this? It's critical to keep really good test coverage to avoid regressions.

@justin808 Sure, I'll work on it this week

@jtibbertsma Keep me posted!

Coverage remained the same at 82.987% when pulling 9b98494 on jtibbertsma:add-new-api-register-renderer into 60d9f7f on shakacode:master.

Coverage remained the same at 82.987% when pulling 6bcf72f on jtibbertsma:add-new-api-register-renderer into 1174ab5 on shakacode:master.

@justin808 I added a simple integration test that shows that the renderer function gets called correctly.

@jtibbertsma REALLY AWESOME. Let's address the comments!

Reviewed 8 of 12 files at r1, 8 of 8 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

CHANGELOG.md, line 8 at r5 (raw file):

## [Unreleased]
##### Added
- New API registerRenderer which allows a user to manually render their app to the DOM by [jtibbertsma](https://github.com/jtibbertsma)

reference the PR, as done below for other changes

you can use the release 6.2.0

and change the bottom part showing the diff

README.md, line 456 at r5 (raw file):

1. [React on Rails docs for react-router](docs/additional-reading/react-router.md)
1. Examples in [spec/dummy/app/views/react_router](spec/dummy/app/views/react_router) and follow to the JavaScript code in the [spec/dummy/client/app/startup/ServerRouterApp.jsx](spec/dummy/client/app/startup/ServerRouterApp.jsx).

we should mention the registerRenderer here, or maybe in the docs for react-router.

README.md, line 476 at r5 (raw file):

  + [Turbolinks](docs/additional-reading/turbolinks.md)

+ **Javascript**

we need to refer to the page on code splitting here

docs/additional-reading/code-splitting.md, line 15 at r5 (raw file):

 (client) <!-- react-empty: 1 -
 (server) <div data-reactroot="

this is formatting so you have to scroll sideways a ton

please see if you can change that

docs/additional-reading/code-splitting.md, line 18 at r5 (raw file):

<!--This comment is here because the comment beginning on line 13 messes up Sublime's markdown parsing-->

Different markup is generated on the client than on the server. Why does this happen? When you register a component with `ReactOnRails.register`, react on rails will render the component as soon as the page loads. However, react-router renders a comment while waiting for the code chunk to be fetched from the server. This means that react will tear all of the server rendered code out of the DOM, and then rerender it a moment later once the code chunk arrives from the server, defeating most of the purpose of server rendering.

I'm a bit confused about the interaction between deferred rendering and react-router. Is the idea that a page is bookmarked, and react-router's deferred rendering counts as a first render, so that the server render and what's in react-router's deferred renders should be the same?

It would be great to have some graphic or other explanation of this.

What's not obvious is that we can server render what the deferred rendering will be.

docs/additional-reading/code-splitting.md, line 26 at r5 (raw file):

Here's an example of how you might use this in practice:

page.html.erb

Maybe use #### to format the file names.

docs/additional-reading/code-splitting.md, line 42 at r5 (raw file):

ReactOnRails.registerStore({applicationStore});
ReactOnRails.register({NavigationApp});
ReactOnRails.registerRenderer({RouterApp});

We need to be very clear that RouterApp will handle calling ReactDOM to create the react element, attached to a DOM node.

Should we be showing some serverRegistration.js page?

docs/additional-reading/code-splitting.md, line 85 at r5 (raw file):

### Caveats

If you're going to try to do code splitting with server rendered routes, it's important that you have seperate webpack configurations for client and server. The code splitting happens for the client, but the server should one big file.

should one big file

maybe:

should be one file containing all the JavaScript code

docs/api/javascript-api.md, line 64 at r5 (raw file):

   * Allows registration of renderers. A renderer is a function that accept three args:
   * props, railsContext, and domNodeId, and is responsible for rendering a component
   * to the DOM. Not available on the server. For one possible use case see:

We might be able to throw an error if this is called on the server since we'll know we're server rendering with the rails context.

node_package/src/ReactOnRails.js, line 114 at r5 (raw file):

    ComponentRegistry.registerRenderer(renderers);
  },

Please move this next to the definition immediately below registerStore

spec/dummy/client/app/startup/ManualRenderAppRenderer.jsx, line 12 at r5 (raw file):

  );

  ReactDOM.render(reactElement, document.getElementById(domNodeId));

I feel that we need the example to show react-router.

We use react-router in the dummy apps.

Another consideration is support for react-router v4.

Comments from Reviewable

I have one comment below.

And also one more comment on the global API: I'm not a big fan of owning key deps (as ReactDOM) w/o strong reason (is there any?), so leaving it in user land by default is IMO a better design decision, as we had already few issues w/ it.

Review status: all files reviewed at latest revision, 12 unresolved discussions.

docs/additional-reading/code-splitting.md, line 85 at r5 (raw file):

### Caveats

If you're going to try to do code splitting with server rendered routes, it's important that you have seperate webpack configurations for client and server. The code splitting happens for the client, but the server should one big file.

I don't think the main point here is different webpack configs (as client/server configs are usually different regardless of whether or not code splitting is used), but the fact that it requires different routes files for the client and the server (w/ same routes, but different components resolvers), which is most uncommon & untransparent part here.

Comments from Reviewable

Review status: 13 of 18 files reviewed at latest revision, 12 unresolved discussions.

docs/additional-reading/code-splitting.md, line 18 at r5 (raw file):

The server render matches the deferred render because the server bundle is a single file, and so it doesn't need to wait for anything to be fetched.

Comments from Reviewable

Review status: 13 of 18 files reviewed at latest revision, 12 unresolved discussions.

docs/api/javascript-api.md, line 64 at r5 (raw file):

Comments from Reviewable

Coverage remained the same at 82.987% when pulling 174a3ea on jtibbertsma:add-new-api-register-renderer into 1174ab5 on shakacode:master.

Review status: 13 of 18 files reviewed at latest revision, 12 unresolved discussions.

spec/dummy/client/app/startup/ManualRenderAppRenderer.jsx, line 12 at r5 (raw file):

Comments from Reviewable

Looking good!

Reviewed 5 of 5 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

docs/additional-reading/code-splitting.md, line 18 at r5 (raw file):

docs/additional-reading/code-splitting.md, line 85 at r5 (raw file):

docs/additional-reading/code-splitting.md, line 58 at r6 (raw file):

});

Note that you should not use registerRenderer on the server. Instead, use register like normal. For an example of how to set up an app for server rendering, see the react router docs.

We'll know if we're server rendering. Could we allow the same registration code for client and server, and just ensure that we don't do the deferred render thing on the server?

Needing this to be different for the server rendering seems like a big gotcha.

Comments from Reviewable

@jtibbertsma I'd like to get this in the next release, maybe this weekend!

Review status: all files reviewed at latest revision, 5 unresolved discussions.

docs/additional-reading/code-splitting.md, line 85 at r5 (raw file):

docs/additional-reading/code-splitting.md, line 58 at r6 (raw file):

Comments from Reviewable

Review status: 17 of 18 files reviewed at latest revision, 5 unresolved discussions.

docs/additional-reading/code-splitting.md, line 18 at r5 (raw file):

Comments from Reviewable

Review status: 17 of 18 files reviewed at latest revision, 5 unresolved discussions.

docs/additional-reading/code-splitting.md, line 58 at r6 (raw file):

This is an important feature that we'll probably use.

The key question for me is if we should have separate registerRenderer and register APIs.

Since this is an API change, I want to make sure we get this right.

Possibly we'll implement this as a beta release to get some community feedback?

Comments from Reviewable

@justin808 I've been thinking about this, and I've realized that there's actually no need to have a new API for this. Instead of having registerRenderer, register could be modified so that it checks the number of arguments of the generator function. If the function takes three arguments, it would be considered a renderer. This is cleaner, because there's no need for a new API, but we would still be able to control the call to ReactDOM.render if we want. What do you think?

@jtibbertsma I look forward to seeing what you come up with!

Coverage remained the same at 82.987% when pulling 48e8daf on jtibbertsma:add-new-api-register-renderer into c2b85c9 on shakacode:master.

Coverage remained the same at 82.987% when pulling 2e5ea27 on jtibbertsma:add-new-api-register-renderer into c2b85c9 on shakacode:master.

Coverage remained the same at 82.987% when pulling 2e5ea27 on jtibbertsma:add-new-api-register-renderer into c2b85c9 on shakacode:master.

Coverage remained the same at 82.987% when pulling c9a3e17 on jtibbertsma:add-new-api-register-renderer into c2b85c9 on shakacode:master.

@jtibbertsma This is looking amazing. Keep me posted when you want more review.

Reviewed 8 of 12 files at r8, 1 of 3 files at r9, 11 of 14 files at r12, 1 of 1 files at r13, 2 of 2 files at r14.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

docs/additional-reading/code-splitting.md, line 22 at r14 (raw file):

### The solution

To prevent this, you have to wait until the code chunk is fetched before doing the initial render on the client side. To accomplish this, react on rails allows you to register a renderer. This works just like registering a generator function, except that the function you pass takes three arguments: `renderer(props, railsContext, domNodeId)`, and is responsible for calling `ReactDOM.render` to render the component to the DOM. React on rails will automatically detect when a generator function takes three arguments, and will not call `ReactDOM.render`, instead allowing you to control the initial render yourself.

Cool! I just hope this is not to subtle.

docs/additional-reading/code-splitting.md, line 51 at r14 (raw file):

import ReactOnRails from 'react-on-rails';
import NavigationApp from './NavigationApp';
import RouterApp from './RouterAppServer';

I'd put a comment above this line and the file above, to highlight the difference!

docs/additional-reading/code-splitting.md, line 60 at r14 (raw file):

});

Note that you should not register a renderer on the server, since there won't be a domNodeId when we're server rendering. For an example of how to set up an app for server rendering, see the react router docs.

and comment here on how RouterApp is two different imports given client vs server.

docs/additional-reading/code-splitting.md, line 101 at r14 (raw file):

The idea is that match from react-router is async; it fetches the component using the getComponent method that you provide with the route definition, and then passes the props to the callback that are needed to do the complete render. Then we do the first render inside of the callback, so that the first render is the same as the server render.

The server render matches the deferred render because the server bundle is a single file, and so it doesn't need to wait for anything to be fetched.

How does this fit into React Router v4?

https://github.com/ReactTraining/react-router/tree/v4

I'm OK making this feature only work with v4.

Or we should make the docs work with v3 and v4.

CC: @alexfedoseev @robwise

docs/additional-reading/code-splitting.md, line 107 at r14 (raw file):

If you're going to try to do code splitting with server rendered routes, you'll probably need to use seperate route definitions for client and server to prevent code splitting from happening for the server bundle. The server bundle should be one file containing all the JavaScript code.

The reason is we do server rendering with ExecJS, which is not capable of doing anything asynchronous. It would be impossible to asyncronously fetch a code chunk while server rendering. See [this issue](https://github.com/shakacode/react_on_rails/issues/477) for a discussion.

FYI -- we're investigating alternatives to execJS. Possibly, @jtibbertsma, we could get you involved.

#625

docs/additional-reading/code-splitting.md, line 109 at r14 (raw file):

The reason is we do server rendering with ExecJS, which is not capable of doing anything asynchronous. It would be impossible to asyncronously fetch a code chunk while server rendering. See [this issue](https://github.com/shakacode/react_on_rails/issues/477) for a discussion.

Also, do not attempt to register a renderer on the server. Instead, register either a generator function or a component. If you register a renderer in the server bundle, you'll get an error when react on rails tries to server render the component.

also point out to have different entry files for the server and client webpack configs

docs/additional-reading/code-splitting.md, line 123 at r14 (raw file):

This causes Webpack to prepend the code chunk filename with /assets/ in the request url. The react on rails sets up the webpack config to put webpack bundles in app/assets/javascripts/webpack, and modifies config/initializers/assets.rb so that rails detects the bundles. This means that when we prepend the request url with /assets/, rails will know what webpack is asking for.

Suppose this is not done. How hard is it to debug this?

Maybe show the error here so that a google search will find this page.

node_package/src/serverRenderReactComponent.js, line 18 at r14 (raw file):

    const componentObj = ComponentRegistry.get(name);
    if (componentObj.isRenderer) {
      throw new Error(`Detected a renderer while server rendering component '${name}'`);

Let's make the error a bit more explicit, even including a doc link.

node_package/tests/ComponentRegistry.test.js, line 59 at r14 (raw file):

  const expected = { name: 'C4', component: C4, generatorFunction: true, isRenderer: true };
  assert.deepEqual(actual, expected,
    'ComponentRegistry registers and retrieves renderers');

How about a test with 2 params?

spec/dummy/client/app/startup/DeferredRenderAppRenderer.jsx, line 3 at r14 (raw file):

import React from 'react';
import ReactDOM from 'react-dom';
import { match, Router, browserHistory } from 'react-router';

how will this change with https://github.com/ReactTraining/react-router/tree/v4 ?

spec/dummy/client/app/startup/DeferredRenderAppServer.jsx, line 38 at r14 (raw file):

};

export default DeferredRenderAppServer;

Let's have some reference to these example files in the doc MD file.

spec/dummy/spec/features/integration_spec.rb, line 176 at r8 (raw file):

#     expect(page).to_not have_errors
#   end
# end

we should uncomment the test

Comments from Reviewable

Review status: all files reviewed at latest revision, 18 unresolved discussions.

docs/additional-reading/code-splitting.md, line 22 at r14 (raw file):

I like this way better than having a separate API.

docs/additional-reading/code-splitting.md, line 51 at r14 (raw file):

Done.

docs/additional-reading/code-splitting.md, line 60 at r14 (raw file):

Done.

docs/additional-reading/code-splitting.md, line 101 at r14 (raw file):

It's really annoying when libraries completely change their APIs. I suppose that all the docs will have to be updated for v4. I don't know how code splitting will work with v4 yet. I'll do some more research on it eventually. Anyway, it's still in alpha, so I'm not going to worry too much about this right now.

docs/additional-reading/code-splitting.md, line 107 at r14 (raw file):

Cool

docs/additional-reading/code-splitting.md, line 109 at r14 (raw file):

Done.

docs/additional-reading/code-splitting.md, line 123 at r14 (raw file):

If you don't put /assets/ as the public path, webpack will send the request to /{filename}, and Rails will handle the request, since NGINX or whatever won't see it as a request for an asset. The exact error you'll get depends on how your Rails router is set up. You'll probably just get a 404 error, but since you're probably using react-router, you might have some sort of catch all route. It depends on what your catch all route does.

Another consideration is how you do error handling with code splitting. With webpack v1, there's really no way to implement error handling, but in v2 you can implement error handling by calling .catch on the Promise returned by System.import. Since we're using v1, I didn't mention error handling.

I went ahead and added part of this explanation and a possible error message that you might get.

node_package/src/serverRenderReactComponent.js, line 18 at r14 (raw file):

Done.

node_package/tests/ComponentRegistry.test.js, line 59 at r14 (raw file):

Do you have something in mind? I can't think of anything to test here other than checking that isRenderer key is set to true.

spec/dummy/client/webpack.client.rails.build.config.js, line 15 at r14 (raw file):

Do you mean that the code chunks should go to a different folder than app-bundle and vendor-bundle?

spec/dummy/client/app/startup/DeferredRenderAppRenderer.jsx, line 3 at r14 (raw file):

I'll have to look into it.

spec/dummy/client/app/startup/DeferredRenderAppServer.jsx, line 38 at r14 (raw file):

Done.

spec/dummy/spec/features/integration_spec.rb, line 176 at r8 (raw file):

Done.

Comments from Reviewable

Coverage remained the same at 82.987% when pulling 15c0618 on jtibbertsma:add-new-api-register-renderer into c2b85c9 on shakacode:master.

This is super close to being ready.

@jtibbertsma please see my comments and let me know if you want me to merge this. This is really an impressive PR!

Reviewed 3 of 3 files at r15.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

docs/additional-reading/code-splitting.md, line 85 at r5 (raw file):

@jtibbertsma We really want the spec/dummy example to show code splitting on the client side. Why? This will show the usefulness of this technique!

docs/additional-reading/code-splitting.md, line 101 at r14 (raw file):

@jtibbertsma The changes are really great and worth the churn. Yes, we will eventually update the docs. I think the technique is the same.

docs/additional-reading/code-splitting.md, line 123 at r14 (raw file):

Webpack v2 is close enough to being published that it's worth considering for the documentation.

node_package/src/serverRenderReactComponent.js, line 20 at r15 (raw file):

      throw new Error(`\
Detected a renderer while server rendering component '${name}'. \
See https://github.com/shakacode/react_on_rails#renderer-functions`);

is this better than the code-spliting.md doc?

node_package/tests/ComponentRegistry.test.js, line 59 at r14 (raw file):

that's exactly what I was thinking of, except, isRenderer should be false

spec/dummy/client/webpack.client.rails.build.config.js, line 15 at r14 (raw file):

Not a separate folder, but we could have app-bundle-router, and app-bundle-route-b.

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions.

docs/additional-reading/code-splitting.md, line 85 at r5 (raw file):

I did put a code splitting example in spec/dummy.

node_package/src/serverRenderReactComponent.js, line 20 at r15 (raw file):

Yeah. The information there is more relevant to the error message, and it links to code-splitting.md

node_package/tests/ComponentRegistry.test.js, line 59 at r14 (raw file):

Ok, I see. I'll add another test

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions.

docs/additional-reading/code-splitting.md, line 123 at r14 (raw file):

Ok, I added a comment about error handling.

node_package/tests/ComponentRegistry.test.js, line 59 at r14 (raw file):

I added another test case.

spec/dummy/client/webpack.client.rails.build.config.js, line 15 at r14 (raw file):

I think this is fine how it is.

Comments from Reviewable

Coverage remained the same at 82.987% when pulling 730bd9b on jtibbertsma:add-new-api-register-renderer into c2b85c9 on shakacode:master.

@justin808 Ready to merge? Should I rebase and squash the commits?

@jtibbertsma If you'd like to create a nice squashed commit with an awesome, that would be super!

Reviewed 2 of 2 files at r16.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

Coverage remained the same at 83.111% when pulling 55ed923 on jtibbertsma:add-new-api-register-renderer into 2a227d4 on shakacode:master.

@justin808 Done

Reviewed 6 of 6 files at r17.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable